Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: dont mutate Story prop types to optional #14550

Closed
wants to merge 2 commits into from
Closed

fix: dont mutate Story prop types to optional #14550

wants to merge 2 commits into from

Conversation

Bram-Zijp
Copy link

@Bram-Zijp Bram-Zijp commented Apr 11, 2021

Issue:
When using TypeScript and you specify props, the props were converted to optional. I don't think this is a good pattern. One should type cast if they want to go against specification. I don't see any reason to make it Partial in the code base (correct me if I'm wrong, this Partial seems intentional).

What I did

Removed the Partial from the type defs in my node_modules folder and saw proper results. I searched this repo for references to BaseAnnotations and found nothing concerning.

How to test

  • Is this testable with Jest or Chromatic screenshots? -> No
  • Does this need a new example in the kitchen sink apps? -> No
  • Does this need an update to the documentation? -> No
  • I don't know what the reasoning behind this Partial is, I don't see a reason to mutate this the Args type to something else. If modules need to alter it, shouldn't they need to be the ones responsible for doing so? Needs to be verified though. This Partial was probably intentional -> Yes

Example

import React from "react";
import { Story } from "@storybook/react";

interface ExampleProps {
  requireMe: "please";
  fine?: true;
}

const Template: Story<ExampleProps> = (args) => <div {...args}>example</div>;

export const Test = Template.bind({});

Test.args = {}; // error requireMe is undefined error should be visible here. This makes it easy to spot mistakes when you make em.

@ndelangen
Copy link
Member

Both Partial and non-Partial make sense.

The Partial was added intentionally with the mindset that users would likely want to supply a Props interface which is very strict. (all sorts of required fields)

But then do this:

import React from "react";
import { Story } from "@storybook/react";

interface ExampleProps {
  requireMe: "please";
  fine?: true;
}

const Template: Story<ExampleProps> = (args) => <div {...defaultProps} {...args}>example</div>;

export const Test = Template.bind({});

Test.args = {}; // no error, because the component will never be called without 'missing' properties (everything is optional)

But a default stricter stance also makes sense...
Right now, we do not allow the user to be strict.

A user could very easily opt-in to all properties being optional by wrapping their type with Partial<> themselves.

I think this is an OK, change. Though it might be a breaking change.

@ndelangen
Copy link
Member

IF we were to make this change...
Here's how a user could modify their story to get the old behavior back:

import React from "react";
import { Story } from "@storybook/react";

interface ExampleProps {
  requireMe: "please";
  fine?: true;
}

- const Template: Story<ExampleProps> = (args) => <div {...defaultProps} {...args}>example</div>;
+ const Template: Story<Partial<ExampleProps>> = (args) => <div {...defaultProps} {...args}>example</div>;

export const Test = Template.bind({});

Test.args = {}; // no error, because the component will never be called without 'missing' properties (everything is optional)

@shilman
Copy link
Member

shilman commented Apr 13, 2021

I think the issue here is that args are cascading. So let's say you have:

interface ExampleProps {
  a: number;
  b: number;
}

You could have stories that look like:

import React from "react";
import { Story, Meta } from "@storybook/react";

interface ExampleProps {
  require1: string;
  require2: string;
}

export default {
  title: 'foo';
  args: { require1: 'haha' }
} as Meta<ExampleProps>

const Template: Story<ExampleProps> = (args) => <div {...args}>example</div>;
export const Test = Template.bind({});
Test.args = {
  require2: 'hoho';
}

Which is valid with the partial type but not valid with the strict type.

@ndelangen
Copy link
Member

Yeah the more I think about the more I think the Partial approach makes the most sense as a default.

We could do this:

import React from "react";
import { StrictStory } from "@storybook/react";

interface ExampleProps {
  requireMe: "please";
  fine?: true;
}

const Template: StrictStory<ExampleProps> = (args) => <div {...args}>example</div>;

export const Test = Template.bind({});

Test.args = {}; // error requireMe is undefined error should be visible here. This makes it easy to spot mistakes when you make em.

@shilman
Copy link
Member

shilman commented Apr 14, 2021

Given the problems with this PR, are there cleaner solutions to help make this more strict? @ndelangen @Bram-Zijp

@ndelangen
Copy link
Member

As is, this can't be merged. But I'd be open to a StrictStory addition.

@yannbf
Copy link
Member

yannbf commented Apr 23, 2021

I'm pretty sure that making the Story type strict will break a lot of people's codes. I think Partial makes sense, given how flexible the args are. I'd vouch for a secondary generic parameter in the Story type, with a sole purpose to toggle a strict mode. This way we introduce a desired feature that users can easily opt in:

type StrictModes = 'loose' | 'strict'
interface Story<Args, StrictMode extends StrictModes = 'loose'> {
  args: StrictMode extends 'strict' ? Args : Partial<Args>
}

const PartialTemplate: Story<ExampleProps> = (args) => <div {...args}>example</div>;
const StrictTemplate: Story<ExampleProps, 'strict'> = (args) => <div {...args}>example</div>;

This way people can refer to tsdoc from Story and figure it out:
image

Here's a playground with a working prototype

Either that or StrictStory type are fine by me!

Edit: thinking more about it, I guess people won't want to have a mix of Story<Props> and Story<props, 'strict'>, so maybe StrictStory type makes more sense and it's easier for people to use.

@Bram-Zijp
Copy link
Author

I agree on that @yannbf. I'm lacking time and intel on the repo. Could someone pick this up?

@MichaelArestad MichaelArestad added this to the 7.0 milestone Aug 16, 2021
@apdrsn
Copy link

apdrsn commented Nov 4, 2021

Hi Team Storybook :)
What's the status on this feature? I think the StrictStory would make sense. It's a problem that we can't rely on the type checking as it is now :)

@ndelangen
Copy link
Member

I think introducing StrictStory would be the best way forwards. Do you agree @yannbf @shilman ?

@theinterned theinterned removed their request for review January 6, 2022 20:15
@ndelangen ndelangen self-assigned this Jul 1, 2022
@nx-cloud
Copy link

nx-cloud bot commented Jul 1, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2ec5775. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ndelangen
Copy link
Member

So with future/base soon to be merged into next we need to make a decision here @tmeasday @shilman...

  • Do we make this strict?
  • Do we go with Kasper's new Typescript syntax
  • Do we add a StrictStoryFn and a StrictStoryObj interface?

@ndelangen ndelangen removed their assignment Aug 23, 2022
@ndelangen ndelangen removed their request for review August 23, 2022 14:39
@ndelangen
Copy link
Member

@kasperpeulen What should we do with this PR considering your recent and ongoing work around types and strictness?
Can we somehow incorporate some of @Bram-Zijp 's commits so they get a bit of credit? I suspect this PR won't actually land.

I'm sorry about that @Bram-Zijp
The solution wasn't as straight forward, but in the end I think you'll be quite pleased with the hard work @kasperpeulen has put into and the results he'll achieve (soon, I'm sure).

It will likely mean your PR will most likely stay unmerged, and at some point will be closed.
If it does, please do know we very much appreciate your time, energy and expertise in creating this PR. Thank you.

@kasperpeulen I'll leave it to you to decide what to do with this PR, let me know if you need any help to decide or to act upon it.

@ndelangen ndelangen marked this pull request as draft October 5, 2022 18:53
@Bram-Zijp
Copy link
Author

@ndelangen I am happy that it got picked up. I appreciate you want to give me some credit but I don't wanna burden anyone. Therefor I am closing this PR. I am a big fan of Storybook, keep up the good work!

@Bram-Zijp Bram-Zijp closed this Oct 5, 2022
@kasperpeulen
Copy link
Contributor

@Bram-Zijp Follow up PR is here:
#19238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants